TST: XFAIL Travis read_html tests #30544
Conversation
alimcmaster1
commented
Dec 29, 2019
•
alimcmaster1
commented
- xref Travis 3.6 Slow Build Test Failures - BS4 Issue #29622
alimcmaster1
added
the
Testing
|
can't make it strict? |
| # https://github.com/pandas-dev/pandas/issues/29622 | ||
| if self.read_html.keywords.get("flavor") == "bs4": | ||
| pytest.xfail("fails for bs4 version >= 4.8.0") | ||
|
|
There was a problem hiding this comment.
did you change this from the decorator in response to my comment? if so, thats not what i meant. the decorator is preferred, but without the strict=False
There was a problem hiding this comment.
Yeah just want to xfail one particular parameterization - bs4 and the parameterization is at class scope - agree decorator is usually preferred.
Updated now - think its easiest to do this in the function itself for this case - unless you know a more elegant way?
There was a problem hiding this comment.
Just seen I can potentially go for the same approach as you did here https://github.com/pandas-dev/pandas/pull/30521/files? Will update unless you think otherwise?
There was a problem hiding this comment.
if the test doesnt use the class fixture(s), could it just be moved out of the class?
If not then the approach in 30521 would be my preference, yes.
There was a problem hiding this comment.
Some class fixtures are used so easier to keep where is. Updated - thanks!
|
looks like there was an xpass on the old_np build. possibly related: i think bs4 had a release yesterday |
Due to the fact we pin bs4 to 4.6.0 in the Other option is to make this not strict? |
@datapythonista preference for this pin?
If all else fails, yah. I appreciate you putting in the effort to avoid this last resort. |
|
Can confirm travis-36-slow build is now passing - https://travis-ci.org/pandas-dev/pandas/jobs/631033419?utm_medium=notification&utm_source=github_status We can remove this from allowed failures in |
We've got bs4 also pinned at 4.6.0 in the minimum versions build. I think ideally we should have a minimum versions build for the slow tests too. I guess if we had it, it'd fail like the old np build. So I guess unpinning will avoid the failure, but won't really fix the problem. Do we want to increase the minimum required version instead? |
Thanks for the info! Should the minimum version build just also run the slow tests opposed to a sep build?
Okay shall we bump to min beautifulsoup4 == 4.8.0 ? ( |
My understanding is that a build with both the "normal" and the slow tests would be too slow. That's why we have the slow marker, so they are called in separate builds.
This is from just few months ago. I think we're usually compatible with dependencies that are at least couple of years old. |
| @@ -13,7 +13,7 @@ dependencies: | |||
| - pytest-azurepipelines | |||
|
|
|||
| # pandas dependencies | |||
| - beautifulsoup4==4.6.0 | |||
| - beautifulsoup4=4.6.0 | |||
There was a problem hiding this comment.
This should be single =
Thanks @datapythonista for the info! Okay so i've adjust the xfail to take into account the version of bs4 installed + removed the travis 36 slow build from the allowed failures. Happy to create a min version build for the slow tests too as you suggested (sep PR). If that's a useful for us? |
I think it makes more sense to discuss and add this as part of #29685. I don't think we really understand well what's being tested. I think creating builds from zero in an organized way is better than add this individually. I'm on it. |
|
All green - @jbrockmendel / @datapythonista mind reviewing? |
|
Thanks @alimcmaster1 |
* XFAIL Tests * XFAIL Tests * Update as per brock suggestion * unpin beautiful soup version * Dont xfail for bs4 < 4.8.0 * Remove slow tests from allowed failures * black pandas
